Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(stdlib): add path package #1065

Merged
merged 1 commit into from
Aug 28, 2023
Merged

feat(stdlib): add path package #1065

merged 1 commit into from
Aug 28, 2023

Conversation

gfanton
Copy link
Member

@gfanton gfanton commented Aug 23, 2023

simply add path package to stdlib, needed by net/url package (and probably some other packages)

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@gfanton gfanton requested review from jaekwon and moul as code owners August 23, 2023 14:20
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Aug 23, 2023
@gfanton gfanton mentioned this pull request Aug 23, 2023
Merged
9 tasks
thehowl
thehowl requested changes Aug 24, 2023
View reviewed changes
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had a look through the Go native implementation. There doesn't seem to be any dependency on packages/features we can't currently support (imports are on errors, utf8 and internal/bytealg, all of which already exist as packages, and I don't see any channels/goros being used).

Could we have this as gno code instead of a native injection?

Also, gnovm/docs/go-gno-compatibility.md should be updated, too. :)

@gfanton
Copy link
Member Author

gfanton commented Aug 24, 2023

I've had a look through the Go native implementation. There doesn't seem to be any dependency on packages/features we can't currently support (imports are on errors, utf8 and internal/bytealg, all of which already exist as packages, and I don't see any channels/goros being used).
Could we have this as gno code instead of a native injection?

Sure, that make sense! I wasn't sure about preferring one location over another. I thought that if no modifications were needed, or if there were only a few methods to add, it would be more efficient to place this directly in the stdlib file rather than creating a specific gno package directory.

@thehowl
Copy link
Member

thehowl commented Aug 24, 2023

I thought that if no modifications were needed, or if there were only a few methods to add, it would be more efficient to place this directly in the stdlib file rather than creating a specific gno package directory.

In general, let's work towards keeping stdlibs.go injections as few as possible. They should be like C extensions in Python: they are possible and sometimes necessary, for either doing impossible/hard things in the VM language (Gno), or because of performance (and then again, isolate specific functions which need them).

Impossible/hard things include, for instance, any use of unsafe (ie. math.Float64frombits), or crypto (crypto/sha256), which is why those are injected.

strconv is currently native but I don't think all methods there are good candidates for native injections. They are still native in #859, but I'd like to change that in the future.

@gfanton gfanton requested a review from a team as a code owner August 24, 2023 16:20
@gfanton gfanton requested a review from thehowl August 25, 2023 06:26
@gfanton
feat: add "path" package
ab50d35 
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
@thehowl thehowl merged commit debf7d9 into gnolang:master Aug 28, 2023
Doozers pushed a commit to Doozers/gno that referenced this pull request Aug 31, 2023
@gfanton @Doozers
feat(stdlib): add path package (gnolang#1065)
510c352 
simply add `path` package to stdlib, needed by `net/url` package (and
probably some other packages)


<details><summary>Contributors' checklist...</summary>

- [X] Added new tests, or not needed, or not feasible
- [ ] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [ ] Updated the official documentation or not needed
- [X] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [ ] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
@moul moul added this to the 🚀 main.gno.land (required) milestone Sep 16, 2023
moul added a commit that referenced this pull request Sep 22, 2023
@gfanton @moul
feat(stdlib): add net/url (#1066)
2ecbb6d 
Added the `net/url` package. This package will be beneficial for
manipulating URLs, for instance, parsing query parameters or for URL
muxing in realms.

depends:
- [x] #1076 
- [x] #1065 

<details><summary>Contributors' checklist...</summary>

- [X] Added new tests, or not needed, or not feasible
- [ ] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [ ] Updated the official documentation or not needed
- [X] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [X] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>
gfanton added a commit to gfanton/gno that referenced this pull request Nov 9, 2023
@gfanton @moul
feat(stdlib): add net/url (gnolang#1066)
5f7141e 
Added the `net/url` package. This package will be beneficial for
manipulating URLs, for instance, parsing query parameters or for URL
muxing in realms.

depends:
- [x] gnolang#1076 
- [x] gnolang#1065 

<details><summary>Contributors' checklist...</summary>

- [X] Added new tests, or not needed, or not feasible
- [ ] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [ ] Updated the official documentation or not needed
- [X] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [X] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thehowl thehowl thehowl approved these changes

@jaekwon jaekwon Awaiting requested review from jaekwon

@moul moul Awaiting requested review from moul

Assignees

@gfanton gfanton

Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
🚀 The Launch [DEPRECATED]
Archived in project
🧐 Review Meeting
Archived in project
Milestone
🚀 Mainnet beta launch
Development

Successfully merging this pull request may close these issues.
3 participants
@gfanton @thehowl @moul